Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open v0.3 API + debiasing #75

Merged
merged 34 commits into from
Jan 2, 2024
Merged

Open v0.3 API + debiasing #75

merged 34 commits into from
Jan 2, 2024

Conversation

splch
Copy link
Contributor

@splch splch commented Jul 24, 2023

This new version of the API separates job status and metadata from actual results into two different endpoints.

v0.3 also includes support for error_mitigation settings like symmetrization as described in https://arxiv.org/pdf/2301.07233.pdf

This includes:

  • add a new error_mitigation parameter on job submission so users can configure it
  • add a new aggregation parameter on results that would allow getting results aggregated under the two different methods described in the paper

@splch splch marked this pull request as ready for review July 24, 2023 06:08
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @splch, thanks for opening this! it all looks great, I've just left some small comments.

pennylane_ionq/api_client.py Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
@timmysilv
Copy link
Contributor

all looks good, but seems like some tests and formatting checks failed (see the CI from my merge commit). once those are fixed, this PR should be good to go!

Also, don't forget to add your name and a quick PR description in the changelog 😄

@splch
Copy link
Contributor Author

splch commented Aug 3, 2023

all looks good, but seems like some tests and formatting checks failed (see the CI from my merge commit). once those are fixed, this PR should be good to go!

Also, don't forget to add your name and a quick PR description in the changelog 😄

hi! the tests should pass now :)

@timmysilv
Copy link
Contributor

bumping the changelog reminder, but otherwise lgtm!

@splch
Copy link
Contributor Author

splch commented Aug 7, 2023

bumping the changelog reminder, but otherwise lgtm!

just got it! thanks for the help :)

@timmysilv
Copy link
Contributor

looks like tests/formatting are still failing ☹️ You can fix formatting by running black -l 100 pennylane_ionq, and the tests can be fixed like this:

diff --git a/tests/test_api_client.py b/tests/test_api_client.py
index e30d8e8..fed274d 100755
--- a/tests/test_api_client.py
+++ b/tests/test_api_client.py
@@ -303,7 +303,7 @@ class TestResourceManager:
         mock_get_response = MockGETResponse(200)
 
         monkeypatch.setattr(
-            requests, "get", lambda url, timeout, headers: mock_get_response
+            requests, "get", lambda url, params, timeout, headers: mock_get_response
         )
         monkeypatch.setattr(
             requests,
diff --git a/tests/test_device.py b/tests/test_device.py
index 328924b..051c038 100755
--- a/tests/test_device.py
+++ b/tests/test_device.py
@@ -121,7 +121,7 @@ class TestDeviceIntegration:
         )
         monkeypatch.setattr(Job, "is_complete", True)
 
-        def fake_response(self, resource_id=None):
+        def fake_response(self, resource_id=None, params=None):
             """Return fake response data"""
             fake_json = {"histogram": {"0": 1}}
             setattr(

Also, can you add one line to the Improvements section of the changelog just mentioning that you've bumped the IonQ api version being used?

It all looks great, happy to approve once those changes are in 😄

CHANGELOG.md Outdated
Comment on lines 17 to 18
Spencer Churchill

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@splch, it might be nice to highlight the error mitigation support in the changelog

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trbromley, combined the two kwargs into a little blurb and moved to "new features", added a link to an IonQ guide for more info

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (82ace77) 95.75% compared to head (36b8c96) 96.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   95.75%   96.50%   +0.75%     
==========================================
  Files           5        5              
  Lines         306      315       +9     
==========================================
+ Hits          293      304      +11     
+ Misses         13       11       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov seems upset with this change, so CI is still failing. Just adding a bit more test coverage for the new lines should do it, but I don't want to hold this PR up any longer. lmk if there's any trouble adding that in, and we can otherwise get this merged!

CHANGELOG.md Outdated Show resolved Hide resolved
pennylane_ionq/device.py Show resolved Hide resolved
pennylane_ionq/api_client.py Show resolved Hide resolved
pennylane_ionq/device.py Outdated Show resolved Hide resolved
@lillian542
Copy link
Contributor

Hi @splch! We’ll be releasing an updated version of this plugin as part of the PennyLane release at the beginning of January, so I’m going to go ahead and wrap up the remaining testing and docstrings things and get this PR merged.

I'll get the changes in this afternoon, and then if you have any edits you would like to make, please add them by Friday. Thanks so much for your work on this!

CHANGELOG.md Outdated Show resolved Hide resolved
gateset="qis",
shots=1024,
backend="harmony",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the default backend to "harmony" because every job submission was raising a warning unless the user specifies "harmony" (or some other backend). Since I believe this was effectively the default before, this will keep that behaviour and not give users warnings (unless they specify None, in which case the warning is justified)

@lillian542 lillian542 merged commit e592b06 into PennyLaneAI:master Jan 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants